Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rt: overhaul task hooks #7197

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

rt: overhaul task hooks #7197

wants to merge 15 commits into from

Conversation

Noah-Kennedy
Copy link
Contributor

This change overhauls the entire task hooks system so that users can propagate arbitrary information between task hook invocations and pass context data between the hook "harnesses" for parent and child tasks at time of spawn.

This is intended to be significantly more extensible and long-term maintainable than the current task hooks system, and should ultimately be much easier to stabilize.

This change overhauls the entire task hooks system so that users can propagate arbitrary information between task hook invocations and pass context data between the hook "harnesses" for parent and child tasks at time of spawn.

This is intended to be significantly more extensible and long-term maintainable than the current task hooks system, and should ultimately be much easier to stabilize.
@github-actions github-actions bot added R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR labels Mar 6, 2025
@Noah-Kennedy
Copy link
Contributor Author

I'll fix CI tomorrow.

use std::sync::Arc;

/// A factory which produces new [`TaskHookHarness`] objects for tasks which either have been
/// spawned in "detached mode" via the builder, or which were spawned from outside the runtime or
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is incorrect, and I will need to fix it

@rcoh
Copy link
Contributor

rcoh commented Mar 6, 2025

Just reading the API, I really like this. I think its an API that allows us to expand in the future (especially if there isn't a huge perf hit by virtue of open-ended XYZContext objects).

I might consider keeping the current hook APIs (but implementing them behind the scenes with the new API). I think, ultimately, they may still be simpler for some use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants